feat: fetch LMS_USER_ID from LMS #584
Conversation
sameenfatima78
changed the title
sameenfatima78
changed the title
license_manager/apps/api/v1/views.py
Outdated
| """" | ||
| Another approach would be to override this property at child class level. | ||
| @cached_property | ||
| def lms_user_id(self): | ||
| try: | ||
| return utils.get_key_from_jwt(self.decoded_jwt, 'user_id') | ||
| except ParseError: | ||
| lms_client = LMSApiClient() | ||
| user_id = lms_client.fetch_lms_user_id(self.request.user.email) | ||
| return user_id | ||
| """ | ||
|
|
There was a problem hiding this comment.
Still need to check if this is the right way to override a property. Stackoverflow answers suggests to override the property setter method instead. The changes worked locally though. https://stackoverflow.com/questions/3336767/overriding-inherited-properties-getters-and-setters-in-python
| def lms_user_id(self): | ||
| return utils.get_key_from_jwt(self.decoded_jwt, 'user_id') | ||
| try: | ||
| return utils.get_key_from_jwt(self.decoded_jwt, 'user_id') |
There was a problem hiding this comment.
[suggestion] It may be worth following the pattern of integrating the lms_user_id as a model field on the core User model, similar to how enterprise-access is set up.
The benefit of this is that the user id is persisted in the database for each User record, which is created whenever an authenticated API request is made to the service.
For example, the EDX_DRF_EXTENSIONS setting can be modified to include lms_user_id (example source) and the lms_user_id field can be added to the core User model (example source).
There was a problem hiding this comment.
Hmm, that said, I suppose this would still assume a JWT cookie with a user_id claim...
There was a problem hiding this comment.
You could use the pattern Adam described above and do something like this:
- Try to lookup the
lms_user_idby email from the model in this service. - If it's not present, then go fetch from the LMS.
- Populate the model record with the fetched lms_user_id.
|
Hi @adamstankiewicz @iloveagent57 Thank you for the feedback. These are great suggestions. Can we incorporate these changes in a follow-up PR as this change is needed on an urgent basis? |
zamanafzal
changed the title
There was a problem hiding this comment.
I think this is fine for now. I did some investigating and I think the “right” way to fix the problem of the lms user id not being in the JWT is:
- modify the base edx oauth client to pass in user_id in the scope list in the access token request payload, and also
- Ensure there’s an ApplicationAccess record in the LMS for the license-manager service worker that says user_id is allowed to be requested when fetching the token.
I can open a ticket and route to you guys.
|
ah I mispoke slightly - the JWT we care about here is the API user's JWT, not the service worker. So we'd have to bake the |
|
@iloveagent57 Thank you for digging into it. That will be a great help if you can open a ticket with all these details. |
Description
We cannot include user_id claim in JWT for external API requests. We get hit by "user_id not found in your jwt" error while calling
bulk-licesne-enrollPOST endpoint. To fix this, we are now fetching user_id from LMS in case if it is not found in JWT.Link to the associated ticket: https://2u-internal.atlassian.net/browse/ENT-8351
Testing considerations
already been performed.
Post-review
Squash commits into discrete sets of changes